Skip to content

fix(codegen): PreallocateBoxes must not shadow a module-level global (#7521) - #7580

Merged
proggeramlug merged 5 commits into
mainfrom
fix/7521-tracecallback-lost-events
Aug 7, 2026
Merged

fix(codegen): PreallocateBoxes must not shadow a module-level global (#7521)#7580
proggeramlug merged 5 commits into
mainfrom
fix/7521-tracecallback-lost-events

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Closes #7521.

Root cause

Not in diagnostics_channel, and not a closure-capture bug. traceCallback is
a bystander — the minimal repro has no traceCallback in it, and no array:

import { tracingChannel } from "node:diagnostics_channel";   // any import at all
{
  const events: string[] = [];
  function t1(a: any) { events.push("fn:" + a); }
  t1("A");
  console.log(JSON.stringify(events));   // node: ["fn:A"]   perry: []
}

Objects, numbers and strings lose the same way — inside t1, the captured
binding simply reads undefined, so acc.n++ throws
Cannot read properties of undefined and events.push(...) silently
no-ops. The import matters only because it makes the file an ES module,
hence strict, which is what selects the affected lowering. Everything else
about the failing gap test (traceCallback, subscribers, arrows, the array)
was incidental.

crates/perry-codegen/src/stmt/mod.rs, emit_preallocate_boxes: it allocated
a heap box — and, decisively, a ctx.locals slot — for every id in a
Stmt::PreallocateBoxes directive, including ids that
codegen/module_globals_emit.rs had already promoted to
@perry_global_<mod>__<id>.

Codegen already has a rule for this collision, spelled out identically at every
read and write site in expr/literals_vars.rs and stmt/let_stmt.rs:

ctx.boxed_vars.contains(id) && !ctx.module_globals.contains_key(id)

The module global wins — it is already the shared, forward-visible, GC-rooted
cell a box would provide. emit_preallocate_boxes was the one place that never
checked, and the stale ctx.locals entry it left behind is consulted before
ctx.module_globals on both the Stmt::Let reuse path (let_stmt.rs:322) and
the LocalGet/LocalSet store paths (literals_vars.rs:642, :814). So the
declaration wrote its value into the local box-pointer slot, the module global
was never stored, and the closure — which reads through the global, with zero
captures — saw the undefined the global was defined with.

The emitted IR, before:

%r2  = call i64 @js_box_alloc_bits(i64 <undefined>)   ; PreallocateBoxes([1])
%r8  = call i64 @js_closure_alloc(ptr @perry_closure_m3_ts__0, i32 0)
%r14 = call i64 @js_array_alloc(i32 0)                ; const events = []
store ... %r16, ptr %r3                               ; -> the box-pointer slot

@perry_global_m3_ts__1 is defined, registered as a GC root, read and written
by the closure — and never stored by main.

Why now

c6ed8175d (#6853) is not the window. lower_strict_block_fn_decls was added
by #7105 (76fa7b4c9, 2026-08-01), which made a bare block at the top level
of any ES module emit PreallocateBoxes for its let/const bindings. That is
the first time a promoted module-level id was ever handed to
emit_preallocate_boxes. The two commits the issue records as PASS
(17c0ff952, c6ed8175d, both 2026-07-30) both predate it, so the recorded
evidence is consistent — no bisect was needed once the diff was in view.

Fix

Skip promoted ids in emit_preallocate_boxes. The global is statically
initialized to TAG_UNDEFINED, which is exactly what a non-TDZ prealloc box
seeds, so nothing is lost. The TDZ variant is skipped too and the reason is
written into the code: module-global reads are a raw load double @g with no
js_box_get_bits choke point, so seeding TAG_TDZ there would leak the
sentinel into arithmetic instead of throwing a ReferenceError — strictly worse
than the undefined a forward read gets today.

Tests

crates/perry-codegen/src/stmt/prealloc_module_global_tests.rs — an in-src
unit test module
, deliberately not crates/*/tests/*.rs, so it runs in the
per-PR cargo-test gate forever rather than only nightly (CLAUDE.md #5960).
The gap test that surfaced this (test_gap_diagchannel_3082_3084_3085_3086.ts)
is in neither gap_snapshot.json nor known_failures.json and parity is
tag-gated, which is how this sat unnoticed since 2026-07-30.

Four tests, and they are sabotage-checked rather than merely green — with the
ctx.module_globals guard reverted:

test with guard guard reverted
the_captured_module_binding_is_promoted_to_a_global (fixture premise) ok ok
a_preallocated_box_does_not_swallow_the_module_global_store ok FAILED
prealloc_is_a_no_op_for_a_promoted_binding ok FAILED
a_function_local_prealloc_still_gets_its_box (#569/#6044 not gutted) ok ok

The store assertion counts store … ptr @perry_global_… lines inside main()
specifically. A plain contains was not enough and was caught doing nothing:
main also takes the global's address for js_gc_register_global_root, which
is emitted whether or not the declaration ever writes the cell, so the first
draft of that test passed under sabotage.

Validation (local — CI has a deep backlog and may not report)

Scope note

The blast radius is wider than one gap test: every ES module with a
top-level { … } block containing a function declaration that reads a
sibling let/const compiled to a silently-empty binding. Nothing threw, so
this class of breakage was invisible except where a test happened to print the
accumulated value.

https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH

Summary by CodeRabbit

  • Bug Fixes

    • Fixed module-global bindings being incorrectly allocated as local boxes, preventing empty or invalid bindings and preserving correct global access.
    • Ensured temporal dead zone handling remains correct for promoted module globals.
    • Preserved heap-box allocation for ordinary function-local captured bindings.
  • Tests

    • Added regression coverage for module-global and captured-binding behavior.
    • Removed an outdated known-failure entry.
  • Chores

    • Updated the documented and package version to 0.5.1328.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: adc6d5f3-aeaf-471c-9ace-bd93a87adf6e

📥 Commits

Reviewing files that changed from the base of the PR and between b5c8e9c and 5b25de5.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • CLAUDE.md
  • Cargo.toml
  • changelog.d/7580-prealloc-boxes-module-global.md
  • crates/perry-codegen/src/stmt/mod.rs
  • crates/perry-codegen/src/stmt/prealloc_module_global_tests.rs
  • test-parity/known_failures.json

📝 Walkthrough

Walkthrough

This change prevents PreallocateBoxes from allocating local boxes for bindings promoted to module globals. It adds IR-level regression tests, removes the stale parity known-failure entry, updates the changelog, and increments the project version.

Changes

Module-global preallocation

Layer / File(s) Summary
Codegen guard and regression coverage
crates/perry-codegen/src/stmt/mod.rs, crates/perry-codegen/src/stmt/prealloc_module_global_tests.rs
emit_preallocate_boxes skips module-global identifiers, including TDZ requests. Regression tests verify global storage and continued box allocation for function-local bindings.
Release metadata and changelog
changelog.d/7580-prealloc-boxes-module-global.md, test-parity/known_failures.json, CLAUDE.md, Cargo.toml
The fix is documented, the stale parity suppression is removed, and the project version changes from 0.5.1327 to 0.5.1328.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: bug, parity

Suggested reviewers: thehypnoo

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/7521-tracecallback-lost-events

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Raw GC-ratchet A/B, both arms same session

Both arms built from one target directory with the identical -p perry -p perry-runtime-static -p perry-stdlib-static set, main arm produced by checking out origin/main's three touched files, measured back to back with gc_ratchet.py measure --repeats 3.

compared 156 cells across 12 probes

SEMANTIC differences (retention + evacuation counters): 1
  12_large_live_set   heap_used_bytes   main=59943752 mine=59946056  (+0.004%)

host-dependent differences (rss/wall): 36
  ... every rss/peak_rss delta within +-0.6%
  ... wall_ms faster on this branch on all 12 probes (-1.27% .. -13.25%)

The one semantic cell is 12_large_live_set.heap_used_bytes, the single cell the ratchet's own probe_override takes out of the gating family as conservative-stack-scan sample noise (#7554 for the gate, #7558 for the cause): documented spread 9,072 B over 36 runs, and this delta is 2,304 B. Every retention and evacuation counter on the other eleven probes is bit-identical, and every probe checksum matches.

check --profile shared_ci reports the same ten gating breaches with the same values on both arms:

probe metric baseline both arms delta
02_survivor_promotion heap_used_bytes 9,418,232 9,678,792 +2.77%
03_cross_gen_writes copied_objects 13,893 8,212 -40.89%
03_cross_gen_writes copied_bytes 990,736 590,688 -40.38%
03_cross_gen_writes promoted_objects 4,752 0 -100.00%
03_cross_gen_writes promoted_bytes 210,736 0 -100.00%
04_dead_after_deep_stack copied_objects 11,268 565 -94.99%
04_dead_after_deep_stack copied_bytes 663,512 44,688 -93.26%
04_dead_after_deep_stack promoted_objects 4,752 10 -99.79%
04_dead_after_deep_stack promoted_bytes 210,744 440 -99.79%
05_closure_capture heap_used_bytes 6,378,392 7,426,960 +16.44%

That last row is #7559 verbatim. It does not move here in either direction — this PR is not its cause and not its fix. The eight two-sided breaches on 03/04 are the "improvements wearing a two-sided band" #7559 describes.

Caveat stated plainly: this pair ran on the shared dev machine (load 42), so only the semantic families — which the harness documents at 0.000% spread and which are machine-independent — carry weight. The wall-time table in the PR body is from the pinned quiet host.

Correction to the issue brief, and one extra change

The brief said the gap test is in neither gap_snapshot.json nor
known_failures.json. That is right for gap_snapshot.json, but
known_failures.json does carry it — added 2026-07-04, "issue": "3082",
"reason": "diagnostics_channel cluster (#3082/#3084/#3085/#3086); standing per #5917.". So the two gates disagreed: the gap gate saw an untriaged failure, and
the tag-gated parity gate saw an accepted one.

That entry is now stale — the feature cluster it was filed for has long since
landed, and with this fix the test byte-matches
node --experimental-strip-types (26.5.1) with exit 0 across all four blocks.
scripts/parity_known_failures.py only computes failures - allowed, so unlike
gc_root_dominance_allowlist.json (where an entry matching nothing is a red
build) a stale entry here never fails — it would silence a future regression of
exactly the bug this PR fixes. Retired in 3dab6cf.

Caveat stated plainly: that verification is macOS/arm64 only. If the test turns
out to fail on Linux for an unrelated reason, it surfaces in the tag-gated
parity job rather than here.

Ralph Küpper added 5 commits August 7, 2026 08:11
`emit_preallocate_boxes` allocated a heap box — and, decisively, a
`ctx.locals` slot — for every id in a `Stmt::PreallocateBoxes` directive,
including ids that `codegen/module_globals_emit.rs` had already promoted to
`@perry_global_<mod>__<id>`.

Every other read/write site in codegen already encodes the rule that the
module global wins over the box (`ctx.boxed_vars.contains(id) &&
!ctx.module_globals.contains_key(id)`). This one did not, and the stale
`ctx.locals` entry it left behind is consulted BEFORE `ctx.module_globals` on
the `Stmt::Let` reuse path (`let_stmt.rs`) and on the `LocalGet`/`LocalSet`
store paths (`expr/literals_vars.rs`). The declaration therefore wrote its
value into the local box-pointer slot; the module global was never stored;
and every closure that reads the binding through the global saw the
`undefined` it was defined with.

Since #7105 added `lower_strict_block_fn_decls`, a bare block at the top
level of any ES module emits `PreallocateBoxes` for the block's `let`/`const`
bindings, so this hit ordinary code:

    { const events = []; function t() { events.push("x") } t(); events.length }

`t()` ran, `events.length` was 0, and nothing threw. Objects, numbers and
strings lost the same way (`acc` read as `undefined` inside `t`).

Fix: skip promoted ids in `emit_preallocate_boxes`. The global is statically
initialized to `TAG_UNDEFINED`, which is exactly what a non-TDZ prealloc box
seeds. The TDZ variant is skipped too — module-global reads are raw `load
double @g` with no `js_box_get_bits` choke point, so seeding `TAG_TDZ` there
would leak the sentinel into arithmetic rather than throwing.

Closes #7521

Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
…l gap test

`test_gap_diagchannel_3082_3084_3085_3086` was suppressed on 2026-07-04 for the
original #3082/#3084/#3085/#3086 feature cluster, which has since landed. With
the PreallocateBoxes fix it byte-matches `node --experimental-strip-types`
(26.5.1) with exit 0 across all four blocks.

`parity_known_failures.py` only computes `failures - allowed`, so a stale entry
never fails — unlike the gc-root-dominance allowlist, where an entry that
matches nothing is a red build. Leaving it would silence a future regression of
exactly the bug just fixed.

Verified on macOS/arm64 only; a platform-specific surprise would surface in the
tag-gated `parity` job.

Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
…p test

The fragment said the test was in neither gap_snapshot.json nor
known_failures.json, while the same fragment (and the diff) retire its
known_failures.json entry. It was absent from gap_snapshot.json and
SUPPRESSED by a stale known_failures.json entry.
@proggeramlug
proggeramlug force-pushed the fix/7521-tracecallback-lost-events branch from 3dab6cf to 5b25de5 Compare August 7, 2026 06:12
@proggeramlug
proggeramlug merged commit 9688bbe into main Aug 7, 2026
6 of 12 checks passed
@proggeramlug
proggeramlug deleted the fix/7521-tracecallback-lost-events branch August 7, 2026 06:12
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Audit before merge — verified independently, merged as v0.5.1328

Reproduced the minimal case on a clean build first, because the root cause
is a long way from the issue's title and worth confirming from scratch:

with    import: perry=[]         node=["fn:A"]     <- broken
without import: perry=["fn:A"]   node=["fn:A"]     <- fine

That is the whole bug in four lines, and it confirms the report's key
reframing: traceCallback is a bystander, and the import matters only because
it makes the file an ES module, hence strict, which selects the affected
lowering. It also explains why the three probes I posted on #7521 were all green
— none of them was a module.

This is much wider than one gap test. Every ES module with a top-level bare
block whose function declaration reads a sibling let/const compiled to a
silently-empty binding. Nothing threw. Six days on main.

Post-fix, verified on my own build: minimal repro correct, and the gap test
byte-identical to node 26.5.1, exit 0, all 23 lines.

Sabotage-verified the new tests myself. Commenting out the
ctx.module_globals.contains_key(id) guard takes exactly the two positive tests
red — a_preallocated_box_does_not_swallow_the_module_global_store and
prealloc_is_a_no_op_for_a_promoted_binding — while the fixture-premise test
and the #569/#6044 "still gets its box" control stay green. That is the right
shape: the controls prove the tests are not passing for a trivial reason.

Worth singling out, because it is the kind of thing that usually ships
unnoticed: the store assertion counts store … ptr @perry_global_… lines
inside main() rather than using contains, because main also takes the
global's address for js_gc_register_global_root — which is emitted whether or
not the declaration ever writes the cell. The first draft of that test passed
under sabotage for that reason. Catching your own vacuous assertion is the
part of this report I trust most.

Gates re-run here: raw_handle_debt 998 (baseline 998), check_file_size.sh
clean, cargo fmt --check clean.

One correction to the fragment, applied before merge. It stated the gap test
was in "neither gap_snapshot.json nor known_failures.json" while the same
fragment — and the diff — retire its known_failures.json entry. It was absent
from gap_snapshot.json and suppressed by a stale known_failures.json entry.
Reworded, since the fragment is the permanent record.

The suppression finding deserved its own ticket and now has one: #7582.
known_failures.json computes failures - allowed, so a stale entry is inert —
it never fails and never asks to be removed. That is a fifth way a gate can be
unable to fail, and the most insidious, because the job is genuinely green and
genuinely running. gc_root_dominance_allowlist.json already solves this in
this repo ("an entry that matches nothing FAILS the build"), so the fix is to
adopt that property, together with #797's per-entry provenance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

regression: tracingChannel().traceCallback loses every events.push (test_gap_diagchannel_3082_3084_3085_3086, unnoticed since 2026-07-30)

1 participant